Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix potential unsafe initialization in the Graph class #606

Merged
merged 7 commits into from
Jul 12, 2024

Conversation

caguero
Copy link
Contributor

@caguero caguero commented Jul 2, 2024

🦟 Bug fix

Partially fixes #269 (Graph class).

Summary

I implemented the "Construct on first use" idiom (https://www.freecodecamp.org/news/cpp-static-initialization-order-fiasco/) to avoid the potential unsafe construction.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.98%. Comparing base (c32e96e) to head (05b847d).
Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #606   +/-   ##
=======================================
  Coverage   95.97%   95.98%           
=======================================
  Files         147      147           
  Lines       10122    10130    +8     
=======================================
+ Hits         9715     9723    +8     
  Misses        407      407           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -150,7 +150,7 @@ namespace graph
{
std::cerr << "[Graph::AddVertex()] The limit of vertices has been "
<< "reached. Ignoring vertex." << std::endl;
return Vertex<V>::NullVertex;
return NullVertex<V>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like an API change; we may need to do it, but let me check if sdformat will be affected

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I restored the original static members and added deprecations. It should be possible to keep using them.
15b8975

Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update the migration guide to document the API break / deprecation, depending on which route we take

@@ -150,7 +150,7 @@ namespace graph
{
std::cerr << "[Graph::AddVertex()] The limit of vertices has been "
<< "reached. Ignoring vertex." << std::endl;
return Vertex<V>::NullVertex;
return NullVertex<V>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -204,9 +205,6 @@ namespace graph
template<typename E>
class UndirectedEdge : public Edge<E>
{
/// \brief An invalid undirected edge.
public: static UndirectedEdge<E> NullEdge;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may choose to make a hard API break here, but I'd prefer to not remove these static types in this pull request, but to deprecate them instead, so that we can coordinate fixes for sdformat and any other gz-* packages in separate pull requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if the changes in 15b8975 are sufficient to keep sdformat and friends compiling.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like sdformat15 will still compile with this change as of 5f606f0 (tested using the ci_matching_branch/ trick partially documented in gazebosim/docs#377)

Build Status https://build.osrfoundation.org/view/gz-ionic/job/sdformat15-install_bottle-homebrew-amd64/210/

@caguero
Copy link
Contributor Author

caguero commented Jul 11, 2024

we should update the migration guide to document the API break / deprecation, depending on which route we take

Migration guide updated. See 15b8975 and 5f606f0.

Migration.md Outdated
+ The functions `Graph::AddEdge()`, `Graph::LinkEdge()`,
`Graph::EdgeFromVertices()` and `Graph::EdgeFromId()` functions might
return NullEdge() instead of NullEdge.
E.g.: https://github.com/gazebosim/gz-math/pull/606/files#diff-0c0220a7e72be70337975433eeddc3f5e072ade5cd80dfb1ac03da233c39c983L222-R222
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these are breaking change in Graph.hh. I had originall commented on Graph.hh because that is where I noticed that our API was changing due to the previously removed type, but for this PR, I think the only needed change to the Migration guide is the deprecations below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, we're not changing the function signature so it should be fine.

Migration.md Outdated Show resolved Hide resolved
caguero and others added 2 commits July 12, 2024 14:22
Co-authored-by: Steve Peters <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
{
static auto e = std::make_unique<EdgeType>(
VertexId_P(kNullId, kNullId), E(), 1.0, kNullId);
return *e;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at Material::Predefined(), I wonder if the unique pointer should be made and released from inside a static lambda function?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to use gz::utils::NeverDestroyed (e.g. https://github.com/gazebosim/sdformat/blob/f05f4e7ad1a6784f9ff1a6c1b362191677baa70d/src/Types.cc#L149). However, the fact that this function returns a non-const reference could be problematic since anyone can modify the value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding the non-const reference, I think the behavior in gz-math7 and earlier with static variables has a similar issue right? I tried changing the NullEdge and NullVertex functions to return a const reference, but lots of Graph.hh functions that currently return non-const references like to return references to NullEdge or NullVertex and fail to compile. This seems like a bigger issue with the API that we could open an issue about.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trying again in #612

scpeters added a commit that referenced this pull request Jul 19, 2024
* Avoid constructor/destructor fiascos in graph class
* Deprecations and migration guide.

Signed-off-by: Carlos Agüero <[email protected]>
Signed-off-by: Carlos Agüero <[email protected]>
Co-authored-by: Steve Peters <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants